Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Turn item hold effects into an enum #5498

Merged
merged 2 commits into from
Oct 10, 2024
Merged

Turn item hold effects into an enum #5498

merged 2 commits into from
Oct 10, 2024

Conversation

Bassoonian
Copy link
Collaborator

Turns hold effects into an enum for easier editing. Also removes a handful of references to functions that no longer exist in battle_ai_util.h.

Discord contact info

bassoonian

mrgriffin
mrgriffin reviewed Oct 10, 2024
View reviewed changes
include/constants/hold_effects.h Outdated
#define HOLD_EFFECT_METAL_POWDER 64
#define HOLD_EFFECT_THICK_CLUB 65
#define HOLD_EFFECT_LEEK 66
enum ItemHoldEffects
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd call this enum ItemHoldEffect. I think this makes more sense when it comes to variables, e.g. Foo(enum ItemHoldEffect effect) takes a single effect, not multiple effects.

I haven't checked, but if the plural naming already has precedent in Expansion I'll try and "correct" them later :)

Suggested change
enum ItemHoldEffects
enum ItemHoldEffect

@AlexOn1ine
Copy link
Collaborator

ItemHoldEffect should be used as a variable identifier in functions where possible

@Bassoonian
Copy link
Collaborator Author

ItemHoldEffect should be used as a variable identifier in functions where possible

I tried this, but I got buried in werror=switch errors because for some reason, supplying a switch with an enum will complain if you don't check each and every enum (or have a default case, I guess)

@mrgriffin
Copy link
Collaborator

I tried this, but I got buried in werror=switch errors because for some reason, supplying a switch with an enum will complain if you don't check each and every enum (or have a default case, I guess)

Just to explain the reason: it's so that if you add a new case to your enum the compiler will point out all the places that you need to update (unless there's a default, as you [afaik] correctly guess).

As a motivating example, consider something like script commands. If we had an enum BattleScriptCommand { BSC_NOP, ... BSC_VARIOUS, ... } then the warning is very likely the right thing, because people rarely write switch (command) and don't want to handle every kind of command.

@AlexOn1ine AlexOn1ine merged commit d0379b9 into rh-hideout:upcoming Oct 10, 2024
1 check passed
@AlexOn1ine AlexOn1ine mentioned this pull request Oct 10, 2024
Open
@mrgriffin
Copy link
Collaborator

I'll PR mrgriffin@c247633 which contains usages of enum ItemHoldEffect once (if?) pret#2043 is accepted.

@AlexOn1ine
Copy link
Collaborator

I'll PR mrgriffin@c247633 which contains usages of enum ItemHoldEffect once (if?) pret#2043 is accepted.

oops, sorry 😅

@Bassoonian Bassoonian deleted the hold_effect_enums branch October 10, 2024 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrgriffin mrgriffin mrgriffin left review comments

@AlexOn1ine AlexOn1ine AlexOn1ine approved these changes

Assignees
No one assigned
Labels
type: cleanup
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Bassoonian @AlexOn1ine @mrgriffin @AsparagusEduardo